HLRC: Add activate watch action by iverase · Pull Request #33988 · elastic/elasticsearch

iverase · 2018-09-24T10:23:12Z

Adds activate watch action to the high level rest client.

Relates #29827

Relates elastic#29827

elasticmachine · 2018-09-24T10:38:31Z

Pinging @elastic/es-core-infra

pcsanwald

A few small naming suggestions and one test suggestion, but I don't want to block merging on re-review.

pcsanwald · 2018-09-28T15:12:55Z

client/rest-high-level/src/main/java/org/elasticsearch/client/watcher/ActivateWatchRequest.java

+    }
+
+    @Override
+    public String toString() {


do we need to override toString here? seems relatively harmless, but wasn't sure if it's necessary.

pcsanwald · 2018-09-28T15:18:07Z

client/rest-high-level/src/test/java/org/elasticsearch/client/WatcherIT.java

+
+        }
+
+        // Activate watch that does not exist


I'd split this into a separate test case, testActivateWatchThatExists and testActivateWatchThatDoesNotExist

pcsanwald · 2018-09-28T15:21:44Z

...nt/rest-high-level/src/test/java/org/elasticsearch/client/WatcherRequestConvertersTests.java

        assertThat(request.getEntity(), nullValue());
    }
+
+    public void testActivateWatch() {


I think testActivateWatchRequestConversion is a clearer name.

hub-cap

Wow im super sorry, i had done a review and forgot to click submit :/

Update to today, and we will very soon have a better way to test responses (you do not have a response test) and to generate docs, in the next day or two. We should hold off until then to merge. Feel free to fix up my nits tho.

hub-cap · 2018-09-25T16:17:03Z

client/rest-high-level/src/main/java/org/elasticsearch/client/WatcherRequestConverters.java

+            .addPathPartAsIs("_xpack")
+            .addPathPartAsIs("watcher")
+            .addPathPartAsIs("watch")
+            .addPathPartAsIs(activateWatchRequest.getWatchId())


.addPathPart(activateWatchRequest...) .addPathPartAsIs("_activate")

Looks like you swapped the two of them.

hub-cap · 2018-09-25T16:18:03Z

client/rest-high-level/src/main/java/org/elasticsearch/client/watcher/ActivateWatchRequest.java

+
+        ValidationException exception = new ValidationException();
+
+        if (watchId == null) {


You should just put these in the constructor (the null check already is) and throw from there instead of here and just not override it

hub-cap · 2018-09-25T16:19:42Z

client/rest-high-level/src/main/java/org/elasticsearch/client/watcher/ActivateWatchRequest.java

+    }
+
+    @Override
+    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {


Node needed and it really doesnt even do anything, so we can remove it and ToXContent interface

hub-cap · 2018-09-25T16:19:56Z

...nt/rest-high-level/src/main/java/org/elasticsearch/client/watcher/ActivateWatchResponse.java

+        return Objects.hash(status);
+    }
+
+


extra space

hub-cap · 2018-09-28T17:03:37Z

client/rest-high-level/src/test/java/org/elasticsearch/client/WatcherIT.java

+
+        }
+
+        // Activate watch that does not exist


hub-cap · 2018-09-28T17:04:32Z

...-high-level/src/test/java/org/elasticsearch/client/documentation/WatcherDocumentationIT.java

        }
    }

+    public void testActivateWatch() throws Exception {


We will have a new ay to deal with the docs+test, we should wait to merge until it is merged.

@hub-cap which is the issue that it is introducing this change?

#34125 and it looks like its merged!

iverase · 2018-10-03T10:17:17Z

@hub-cap Added response test and changed the docs. I think it is getting closer.

Is this change required in 6.x?

hub-cap · 2018-10-03T18:02:30Z

Yea we backport all of the HLRC APIs to the current 6.x. Some of them landed in 6.4, most will land in 6.5

hub-cap

minor nits, then

hub-cap · 2018-10-03T18:03:13Z

client/rest-high-level/src/main/java/org/elasticsearch/client/watcher/ActivateWatchRequest.java

+/**
+ * A request to explicitly activate a watch.
+ */
+public final class ActivateWatchRequest implements Validatable, Closeable {


Any reason this is Closeable?

You may also have a look at TimedRequest, assuming this needs the timeouts (I dont know if it does or not, you would have to research the transport layer action)

hub-cap · 2018-10-03T18:05:37Z

client/rest-high-level/src/main/java/org/elasticsearch/client/watcher/ActivateWatchRequest.java

+
+    public ActivateWatchRequest(String watchId) {
+        if (watchId == null) {
+            throw new IllegalArgumentException("Watch identifier is required");


there is a Objects.requireNonNull helper you can use here

hub-cap · 2018-10-03T18:05:58Z

...nt/rest-high-level/src/main/java/org/elasticsearch/client/watcher/ActivateWatchResponse.java

+        return Objects.hash(status);
+    }
+
+    private static final ParseField STATUS_FIELD = new ParseField("status");


This is typically on teh top of these classes.

hub-cap · 2018-10-03T18:06:13Z

...nt/rest-high-level/src/main/java/org/elasticsearch/client/watcher/ActivateWatchResponse.java

+        new ConstructingObjectParser<>("activate_watch_response", true,
+            a -> new ActivateWatchResponse((WatchStatus) a[0]));
+
+    static {


this too is on top

iverase · 2018-10-04T05:18:59Z

@elasticmachine test this please

iverase · 2018-10-04T06:29:16Z

@elasticmachine test this please

iverase · 2018-10-04T10:52:13Z

@hub-cap The commit cannot be back ported as WatchStatus has not been back ported. If necessary I can coordinate with @jtibshirani to do so.

jtibshirani · 2018-10-04T12:01:26Z

Just backported it to 6.x in 83e30ee, sorry for the delay!

* HLRC: Add activate watcher action Adds activate watch action to the high level rest client. Relates #29827

* master: (25 commits) HLRC: ML Adding get datafeed stats API (elastic#34271) Small fixes to the HLRC watcher documentation. (elastic#34306) Tasks: Document that status is not semvered (elastic#34270) HLRC: ML Add preview datafeed api (elastic#34284) [CI] Fix bogus ScheduleWithFixedDelayTests.testRunnableRunsAtMostOnceAfterCancellation Fix error in documentation for activete watch SCRIPTING: Terms set query expression (elastic#33856) Logging: Drop remaining Settings log ctor (elastic#34149) [ML] Remove unused last_data_time member from Job (elastic#34262) Docs: Allow skipping response assertions (elastic#34240) HLRC: Add activate watch action (elastic#33988) [Security] Multi Index Expression alias wildcard exclusion (elastic#34144) [ML] Label anomalies with multi_bucket_impact (elastic#34233) Document smtp.ssl.trust configuration option (elastic#34275) Support PKCS#11 tokens as keystores and truststores (elastic#34063) Fix sporadic failure in NestedObjectMapperTests [Authz] Allow update settings action for system user (elastic#34030) Replace version with reader cache key in IndicesRequestCache (elastic#34189) [TESTS] Set SO_LINGER and SO_REUSEADDR on the mock socket (elastic#34211) Security: upgrade unboundid ldapsdk to 4.0.8 (elastic#34247) ...

* rename-ccr-stats: (25 commits) HLRC: ML Adding get datafeed stats API (elastic#34271) Small fixes to the HLRC watcher documentation. (elastic#34306) Tasks: Document that status is not semvered (elastic#34270) HLRC: ML Add preview datafeed api (elastic#34284) [CI] Fix bogus ScheduleWithFixedDelayTests.testRunnableRunsAtMostOnceAfterCancellation Fix error in documentation for activete watch SCRIPTING: Terms set query expression (elastic#33856) Logging: Drop remaining Settings log ctor (elastic#34149) [ML] Remove unused last_data_time member from Job (elastic#34262) Docs: Allow skipping response assertions (elastic#34240) HLRC: Add activate watch action (elastic#33988) [Security] Multi Index Expression alias wildcard exclusion (elastic#34144) [ML] Label anomalies with multi_bucket_impact (elastic#34233) Document smtp.ssl.trust configuration option (elastic#34275) Support PKCS#11 tokens as keystores and truststores (elastic#34063) Fix sporadic failure in NestedObjectMapperTests [Authz] Allow update settings action for system user (elastic#34030) Replace version with reader cache key in IndicesRequestCache (elastic#34189) [TESTS] Set SO_LINGER and SO_REUSEADDR on the mock socket (elastic#34211) Security: upgrade unboundid ldapsdk to 4.0.8 (elastic#34247) ...

* HLRC: Add activate watcher action Adds activate watch action to the high level rest client. Relates #29827

HLRC: Add activate watcher action

8c4487d

Relates elastic#29827

iverase requested a review from hub-cap September 24, 2018 10:23

elasticmachine mentioned this pull request Sep 24, 2018

Java high-level REST client completeness for xpack APIs #29827

Closed

98 tasks

iverase changed the title ~~HLRC: Add activate watcher action~~ HLRC: Add activate watch action Sep 24, 2018

remove unused imports

8533941

iverase added :Core/Java High Level REST Client :Distributed/Watcher labels Sep 24, 2018

iverase added v7.0.0 v6.5.0 labels Sep 24, 2018

iverase self-assigned this Sep 24, 2018

Merge branch 'master' into activateWatcher

0a27785

pcsanwald approved these changes Sep 28, 2018

View reviewed changes

hub-cap suggested changes Sep 28, 2018

View reviewed changes

not-napoleon mentioned this pull request Oct 1, 2018

HLRC: Deactivate Watch API #34192

Merged

iverase added 7 commits October 2, 2018 16:43

Implemented first round of PR comments

95c874a

rename test

cdd2579

Merge remote-tracking branch 'origin/master' into activateWatcher

70dcf64

Update docs and cos test using new approach

4a3db30

merge with head and resolve conflicts

b1bd977

remove extra space

04ed79a

add response test

78f88d0

hub-cap approved these changes Oct 3, 2018

View reviewed changes

iverase added 2 commits October 3, 2018 21:39

clean up request and response classes

7799655

remove unused import

d602424

Merge branch 'master' into activateWatcher

17705f2

iverase merged commit 3ccb7af into elastic:master Oct 4, 2018

iverase added a commit that referenced this pull request Oct 4, 2018

HLRC: Add activate watch action (#33988)

d967725

* HLRC: Add activate watcher action Adds activate watch action to the high level rest client. Relates #29827

iverase deleted the activateWatcher branch October 4, 2018 12:47

colings86 added >enhancement and removed :Distributed/Watcher labels Oct 25, 2018

kcm pushed a commit that referenced this pull request Oct 30, 2018

HLRC: Add activate watch action (#33988)

0112631

* HLRC: Add activate watcher action Adds activate watch action to the high level rest client. Relates #29827

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019


		ValidationException exception = new ValidationException();

		if (watchId == null) {

Conversation

iverase commented Sep 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Sep 24, 2018

Uh oh!

pcsanwald left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hub-cap left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iverase commented Oct 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hub-cap commented Oct 3, 2018

Uh oh!

hub-cap left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iverase commented Oct 4, 2018

Uh oh!

iverase commented Oct 4, 2018

Uh oh!

iverase commented Oct 4, 2018

Uh oh!

jtibshirani commented Oct 4, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

iverase commented Sep 24, 2018 •

edited

Loading

iverase commented Oct 3, 2018 •

edited

Loading